feat(stream): compact stream chunk by dropping useless rows in sink #11070

xx01cyx · 2023-07-19T10:39:59Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR compacts a stream chunk by converting UPDATE into INSERT & DELETE, as well as dropping useless rows. Specifically, UPDATE K->K will be dropped. The chunk compaction is used in sink executor and project executor.

Related issues: #10949
Fix #10853

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR contains user-facing changes.

Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

Installation and deployment
Connector (sources & sinks)
SQL commands, functions, and operators
RisingWave cluster configuration changes
Other (please specify in the release note below)

Release note

github-actions

license-eye has totally checked 3827 files.

Valid	Invalid	Ignored	Fixed
1714	1	2112	0

Click to see the invalid file list

src/stream/src/common/compact_chunk.rs

src/stream/src/common/compact_chunk.rs

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…avelabs/risingwave into cyx/compact-stream-chunk

codecov · 2023-07-20T14:48:58Z

Codecov Report

Merging #11070 (7ef5945) into main (da89875) will increase coverage by 0.03%.
Report is 69 commits behind head on main.
The diff coverage is 90.26%.

@@            Coverage Diff             @@
##             main   #11070      +/-   ##
==========================================
+ Coverage   69.76%   69.80%   +0.03%     
==========================================
  Files        1404     1405       +1     
  Lines      235006   235243     +237     
==========================================
+ Hits       163958   164204     +246     
+ Misses      71048    71039       -9

Flag	Coverage Δ
rust	`69.80% <90.26%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/common/src/array/mod.rs	`88.06% <ø> (ø)`
src/common/src/row/project.rs	`74.13% <0.00%> (-5.87%)`	⬇️
src/connector/src/sink/kafka.rs	`38.82% <0.00%> (ø)`
src/connector/src/sink/kinesis.rs	`0.00% <0.00%> (ø)`
src/connector/src/source/external.rs	`35.51% <0.00%> (ø)`
src/frontend/src/optimizer/plan_node/stream.rs	`13.69% <ø> (ø)`
src/stream/src/from_proto/sink.rs	`0.00% <0.00%> (ø)`
src/connector/src/sink/mod.rs	`37.37% <50.00%> (ø)`
src/stream/src/executor/sink.rs	`91.74% <87.50%> (+0.12%)`	⬆️
src/common/src/array/stream_chunk.rs	`77.04% <93.33%> (+15.33%)`	⬆️
... and 5 more

... and 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/common/src/array/compact_chunk.rs

src/common/src/array/data_chunk_iter.rs

TennyZhuang · 2023-07-24T08:00:17Z

A quick question. Should we compact per chunk? Why not per barrier in SinkExecutor?

src/common/src/array/compact_chunk.rs

st1page · 2023-09-12T05:11:26Z

implement some new logic to maintain the UpdateDelete and UpdateInsert in the input chunk. PTAL again @fuyufjh , thanks. Maybe we can open it in project executor?

fuyufjh · 2023-09-12T05:21:17Z

implement some new logic to maintain the UpdateDelete and UpdateInsert in the input chunk. PTAL again @fuyufjh , thanks. Maybe we can open it in project executor?

-1. It seems to be too heavy (and also obscure) for being used in ProjectExecutor, especially for the event reordering

fuyufjh · 2023-09-12T05:25:44Z

From my point of view, this PR has been diverged a lot from its original motivation (#10853), and has become somehow over-designed.

Also slightly -1 for #10949, because it's a problem imagined by ourselves. Even if we need to solve it, we need to carefully choose the implementation (must be very fast) and where to put it on (such as before UDF and after column-pruning Project)

I tend to close this PR now and restart from the very beginning: solve #10853 at SinkExecutor.

st1page · 2023-09-12T05:34:35Z

I tend to close this PR now and restart from the very beginning: solve #10853 at SinkExecutor.

Fine, Let's merge this PR and I will solve it in sink executor later

Co-authored-by: TennyZhuang <zty0826@gmail.com>

…risingwave into cyx/compact-stream-chunk

TennyZhuang

Loooks good enough to merge

fuyufjh · 2023-09-12T06:35:43Z

I tend to close this PR now and restart from the very beginning: solve #10853 at SinkExecutor.

Fine, Let's merge this PR and I will solve it in sink executor later

Misunderstood. 😇 I mean close, not merge.

st1page · 2023-09-12T06:38:55Z

solve #10853 at SinkExecutor

solve #10853 at SinkExecutor need this PR absolutely

fuyufjh · 2023-09-12T06:42:28Z

Anyway, you can merge it if you insist, because it only changes the Sink. Although I think my proposal is a little bit better in terms of efficiency.

It's still unacceptable to me to do the same thing in ProjectExecutor. The reason has been states in previous comments:

I do think it might be dangerous to reorder events in the process of streaming. After all, no operator does this now. But sorry I can't come up with a good example now.

I hope to preserve the U-/U+ rows. There are still lots of operators can benefit more or less from this. Reordering all deletes before inserts will completely break U-/U+.

st1page · 2023-09-12T06:44:15Z

Anyway, you can merge it if you insist, because it only changes the Sink. Although I think my proposal is a little bit better in terms of efficiency.

It's still unacceptable to me to do the same thing in ProjectExecutor. The reason has been states in previous comments:

I do think it might be dangerous to reorder events in the process of streaming. After all, no operator does this now. But sorry I can't come up with a good example now.

I hope to preserve the U-/U+ rows. There are still lots of operators can benefit more or less from this. Reordering all deletes before inserts will completely break U-/U+.

But both issues are solved now 😇 The only remind issue here is the performance overhead now,

fuyufjh · 2023-09-12T07:18:19Z

Anyway, you can merge it if you insist, because it only changes the Sink. Although I think my proposal is a little bit better in terms of efficiency.
It's still unacceptable to me to do the same thing in ProjectExecutor. The reason has been states in previous comments:

I do think it might be dangerous to reorder events in the process of streaming. After all, no operator does this now. But sorry I can't come up with a good example now.

I hope to preserve the U-/U+ rows. There are still lots of operators can benefit more or less from this. Reordering all deletes before inserts will completely break U-/U+.

But both issues are solved now 😇 The only remind issue here is the performance overhead now,

Sorry, I missed some changes in the file. Just had a huddle with @st1page and here is the notes:

~~I hope to preserve the U-/U+ rows. There are still lots of operators can benefit more or less from this. Reordering all deletes before inserts will completely break U-/U+.~~

This doesn't apply on current implementation.

I do think it might be dangerous to reorder events in the process of streaming. After all, no operator does this now. But sorry I can't come up with a good example now.

There is no reordering now. This PR finds out all pair of deletes and insert and set their invisibility to false. I have concern with this behavior, because I'm not 100% sure it's safe.

For example, assume there is a unique(v) constraint in a MView as well as its input stream.

   k   v
--------
+  1   5
-  1   5  <-- to eliminate
+  2   5
-  2   5
+  1   5  <-- to eliminate

If we eliminate the rows, then after + 1 5 and + 2 5, the constraint will be broken temporarily.

In order words, this is somehow another kind of reordering: logically, it delays the delete to the position right before its paired insert.

I know we don't have unique constraint and it's not a good example, but I do have such concern. After all, it's hard to say how much it can improve.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: st1page <1245835950@qq.com> Co-authored-by: stonepage <40830455+st1page@users.noreply.github.com> Co-authored-by: Bugen Zhao <i@bugenzhao.com> Co-authored-by: TennyZhuang <zty0826@gmail.com>

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

compact stream chunk

ad24c38

github-actions bot added the type/feature label Jul 19, 2023

github-actions bot reviewed Jul 19, 2023

View reviewed changes

src/stream/src/common/compact_chunk.rs Outdated Show resolved Hide resolved

Update src/stream/src/common/compact_chunk.rs

132fb6b

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

BugenZhao self-requested a review July 20, 2023 04:02

xx01cyx requested a review from st1page July 20, 2023 05:32

xx01cyx added 4 commits July 20, 2023 06:58

Merge remote-tracking branch 'origin/main' into cyx/compact-stream-chunk

8b3ec22

restore update in kafka debezium

35e12a0

Merge branch 'cyx/compact-stream-chunk' of https://github.com/risingw…

54a5ad5

…avelabs/risingwave into cyx/compact-stream-chunk

risedev c

2bdfe48

BugenZhao reviewed Jul 24, 2023

View reviewed changes

src/common/src/array/compact_chunk.rs Outdated Show resolved Hide resolved

src/common/src/array/data_chunk_iter.rs Outdated Show resolved Hide resolved

st1page linked an issue Jul 24, 2023 that may be closed by this pull request

perf(streaming): compact useless intermediate result in stream chunk #10949

Closed

2 tasks

st1page reviewed Jul 24, 2023

View reviewed changes

src/common/src/array/compact_chunk.rs Outdated Show resolved Hide resolved

xx01cyx and others added 15 commits July 26, 2023 06:46

use stream key as merge key

573524b

update sink and project executors

51bb6f3

fix project hash

bd68db6

fix

d34154b

fix project ut

7e7cfc9

add hint in project

2346f49

rename project methods

4b978f0

clippy

935eb68

fix sink test

5f7c7e2

use prehashed map

51f2c8d

merge main and resolve conflicts

e46c84b

Merge branch 'main' into cyx/compact-stream-chunk

7c3a330

conflick

890f56f

add VisMut

fa27338

add VisMut

d87c0af

fix clippy

57f3317

st1page and others added 3 commits September 12, 2023 13:35

Update src/common/src/array/compact_chunk.rs

05ebe68

Co-authored-by: TennyZhuang <zty0826@gmail.com>

change interface

3fcf00c

Merge branch 'cyx/compact-stream-chunk' of github.com:risingwavelabs/…

7cc30ac

…risingwave into cyx/compact-stream-chunk

TennyZhuang approved these changes Sep 12, 2023

View reviewed changes

st1page added 3 commits September 13, 2023 13:52

fix sink

27d3bc5

try fix

77d2cad

fix

7ef5945

st1page closed this Sep 13, 2023

st1page reopened this Sep 13, 2023

st1page enabled auto-merge September 13, 2023 06:39

st1page added this pull request to the merge queue Sep 13, 2023

st1page changed the title ~~feat(stream): compact stream chunk by dropping useless rows~~ feat(stream): compact stream chunk by dropping useless rows in sink Sep 13, 2023

Merged via the queue into main with commit 690a3b4 Sep 13, 2023
16 of 17 checks passed

st1page deleted the cyx/compact-stream-chunk branch September 13, 2023 07:11

st1page mentioned this pull request Sep 13, 2023

refactor(jdbc-sink): execute statements in batch and set isolation level to RC #12250

Merged

8 tasks

github-merge-queue bot pushed a commit that referenced this pull request Oct 9, 2023

refactor(common): refine Row changes in #11070 (#12652)

bfdafe1

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

st1page mentioned this pull request Nov 14, 2023

perf(streaming): compact useless intermediate result in stream chunk #10949

Closed

2 tasks

StrikeW mentioned this pull request Jun 7, 2024

feat(sink): Support Mongodb sink #17102

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stream): compact stream chunk by dropping useless rows in sink #11070

feat(stream): compact stream chunk by dropping useless rows in sink #11070

xx01cyx commented Jul 19, 2023 •

edited

Loading

github-actions bot left a comment

codecov bot commented Jul 20, 2023 •

edited

Loading

TennyZhuang commented Jul 24, 2023

st1page commented Sep 12, 2023 •

edited

Loading

fuyufjh commented Sep 12, 2023 •

edited

Loading

fuyufjh commented Sep 12, 2023

st1page commented Sep 12, 2023

TennyZhuang left a comment

fuyufjh commented Sep 12, 2023

st1page commented Sep 12, 2023

fuyufjh commented Sep 12, 2023

st1page commented Sep 12, 2023

fuyufjh commented Sep 12, 2023 •

edited

Loading

feat(stream): compact stream chunk by dropping useless rows in sink #11070

feat(stream): compact stream chunk by dropping useless rows in sink #11070

Conversation

xx01cyx commented Jul 19, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Types of user-facing changes

Release note

github-actions bot left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 20, 2023 • edited Loading

Codecov Report

TennyZhuang commented Jul 24, 2023

st1page commented Sep 12, 2023 • edited Loading

fuyufjh commented Sep 12, 2023 • edited Loading

fuyufjh commented Sep 12, 2023

st1page commented Sep 12, 2023

TennyZhuang left a comment

Choose a reason for hiding this comment

fuyufjh commented Sep 12, 2023

st1page commented Sep 12, 2023

fuyufjh commented Sep 12, 2023

st1page commented Sep 12, 2023

fuyufjh commented Sep 12, 2023 • edited Loading

xx01cyx commented Jul 19, 2023 •

edited

Loading

codecov bot commented Jul 20, 2023 •

edited

Loading

st1page commented Sep 12, 2023 •

edited

Loading

fuyufjh commented Sep 12, 2023 •

edited

Loading

fuyufjh commented Sep 12, 2023 •

edited

Loading